Update inspector report file counts and output when 0 issues detected #629
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #629 +/- ##
==========================================
+ Coverage 73.03% 80.51% +7.47%
==========================================
Files 47 47
Lines 1587 1606 +19
==========================================
+ Hits 1159 1293 +134
+ Misses 428 313 -115
🚀 New features to boost your workflow:
|
| self.nmessages = len(messages) | ||
| self.nfiles = len(set(message.file_path for message in messages)) # type: ignore | ||
| self.nfiles_with_issues = len(set(message.file_path for message in messages)) # type: ignore | ||
| self.nfiles_detected = nfiles_detected if nfiles_detected is not None else self.nfiles_with_issues |
There was a problem hiding this comment.
This logic is odd to me. Could we just make nfiles_detected required? I suppose that is a breaking change. Are there other reasons not to make it required?
There was a problem hiding this comment.
I was thinking of the case when a user is calling the format_messages function directly in the Python API and it feels like an extra unnecessary step to ask them to calculate and provide the n_files_detected. The CLI automatically calculates it for all potential cases.
Maybe it would be best to keep optional, but if nfiles_detected is not provided then just do not add any text about the number of files scanned to the output message since listing n_files_with_issues is potentially misleading?
There was a problem hiding this comment.
That makes sense. I like your suggestion and would prefer that.
The current phrasing without the number of files detected is still confusing. The "over N files" suggests those are all of the files that were scanned. Maybe something like "Found N files with issues and M issues total across files." would be clearer? What do you think?
Motivation
Fix #385. Fix #359.
Adds the total number of files scanned to the report and provides a specific message if no issues were detected.
Also fixes an extra file being counted by updating the file path message when
check_unique_identifiersis raised. The original issue mentioned maybe displaying the number of directories, but I think this could potentially lead to more confusion with NWB Zarr files. I thinkcheck_unique_identifiersis also a slightly unique case that is performed at the folder level before other checks.todo: